compiler: gate `extern "{abi}"` in ast_lowering #136603

workingjubilee · 2025-02-05T20:00:09Z

I don't believe low-level crates like rustc_abi should have to know or care about higher-level concerns like whether the ABI string is stable for users. These implementation details can be made less open to public inspection. This way the code that governs stability is near the code that enforces stability, and compiled together.

It also abstracts away certain error messages instead of constantly repeating them.

A few error messages are simply deleted outright, instead of made uniform, because they are either too dated to be useful or redundant with other diagnostic improvements we could make. These can be pursued in followups: my first concern was making sure there wasn't unnecessary diagnostics-related code in rustc_abi, which is not well-positioned to understand what kind of errors are going to be generated based on how it is used.

r? @ghost

compiler/rustc_ast_lowering/src/stability.rs

bors · 2025-02-06T10:44:53Z

☔ The latest upstream changes (presumably #136613) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot · 2025-02-07T19:32:56Z

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in compiler/rustc_codegen_ssa/src/codegen_attrs.rs

cc @jdonszelmann

These commits modify compiler targets.
(See the Target Tier Policy.)

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

workingjubilee · 2025-02-07T19:39:26Z

r? @compiler-errors or reroll

workingjubilee · 2025-02-07T19:57:59Z

I split out the first four commits again into #136706 but that's as far as things can be cut.

bors · 2025-02-09T13:53:29Z

☔ The latest upstream changes (presumably #136762) made this pull request unmergeable. Please resolve the merge conflicts.

compiler-errors · 2025-02-09T20:37:54Z

tests/ui/intrinsics/incorrect-transmute.stderr

@@ -1,4 +1,4 @@
-error[E0658]: intrinsics are subject to change
+error[E0658]: rust-intrinsic ABI is an implementation detail and perma-unstable


nit, what about:

the "rust-intrinsic" ABI is [...]

and similarly for above

Sure! should it be extern "abi" so it looks like what people actually write in syntax?

pushed a diff for consideration~

oops yep sounds good i was busy watching sportsball

compiler-errors

LGTM, i have a few questions but r=me regardless

compiler-errors · 2025-02-09T20:38:30Z

tests/ui/abi/riscv-discoverability-guidance.riscv32.stderr

   |        help: did you mean: `"riscv-interrupt-m"`
   |
   = note: invoke `rustc --print=calling-conventions` for a full list of supported calling conventions
-   = note: please use one of riscv-interrupt-m or riscv-interrupt-s for machine- or supervisor-level interrupts, respectively


where'd this message go? 🤔

It's functionally redundant with this one:

| help: did you mean: "riscv-interrupt-m"

If there are two suggestions of equal distance and we want to suggest "try either of these", then we should just do that in a generalized way instead.

compiler-errors · 2025-02-10T03:58:08Z

tests/ui/intrinsics/incorrect-transmute.stderr

@@ -1,4 +1,4 @@
-error[E0658]: intrinsics are subject to change
+error[E0658]: rust-intrinsic ABI is an implementation detail and perma-unstable


oops yep sounds good i was busy watching sportsball

compiler-errors · 2025-02-10T04:03:04Z

tests/ui/abi/removed-wasm-abi.rs

@@ -1,4 +0,0 @@
-extern "wasm" fn test() {}


why remove this test?

It's a suggestion that was for migrating off a nightly feature, starting from last July.

The only users of the feature that I'm aware of are people implementing next-generation wasm toolchain components. So, the people working with us on the migration of wasm's extern "C" to match clang's. They... have known about this for some time, so it seems unlikely they will still benefit from a continued reminder into the future.

So, I removed the suggestion, as we don't usually keep around migration stuff for nightly things for more than a couple of releases, and it's been some. Thus, this test winds up testing nothing.

By moving this stability check into AST lowering, we effectively make it impossible to accidentally miss, as it must happen to generate HIR. Also, we put the ABI-stability code next to code that actually uses it! This allows code that wants to reason about backend ABI implementations to stop worrying about high-level concerns like syntax stability, while still leaving it as the authority on what ABIs actually exist. It also makes it easy to refactor things to have more consistent errors. For now, we only apply this to generalize the existing messages a bit.

These are either residue of a long-term migration away from something, or are simply trying too hard to be specifically useful: nearest-match suggestions for ABI strings should handle this.

workingjubilee · 2025-02-10T05:25:13Z

@bors r=compiler-errors

bors · 2025-02-10T05:25:15Z

📌 Commit cd9d39e has been approved by compiler-errors

It is now in the queue for this repository.

RalfJung · 2025-02-18T10:40:06Z

compiler/rustc_ast_lowering/src/stability.rs

+    }
+}
+
+pub fn extern_abi_stability(abi: ExternAbi) -> Result<(), UnstableAbi> {


Having this in the middle of this huge crate will make it even harder to get "all relevant info fir a given ABI" at a glance. It's a right mess and hard to keep track of: list of ABIs, stability, adjustments, lowering to CallConv, "is this ABI supported" all happen in completely different parts of the codebase.

We have that issue for all stability concerns, because we have a really bad organization of how stability checks work.

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 5, 2025

workingjubilee force-pushed the move-abi-versioning-into-ast branch from 55bc9eb to 4e4fa2a Compare February 5, 2025 20:00

fbstj reviewed Feb 5, 2025

View reviewed changes

compiler/rustc_ast_lowering/src/stability.rs Outdated Show resolved Hide resolved

workingjubilee force-pushed the move-abi-versioning-into-ast branch 4 times, most recently from c66f564 to 8ebebd3 Compare February 6, 2025 00:29

This comment has been minimized.

Sign in to view

workingjubilee force-pushed the move-abi-versioning-into-ast branch from 8ebebd3 to 901945e Compare February 6, 2025 01:59

workingjubilee force-pushed the move-abi-versioning-into-ast branch from 901945e to 2bd2dea Compare February 7, 2025 19:31

rustbot added the A-attributes Area: Attributes (`#[…]`, `#![…]`) label Feb 7, 2025

workingjubilee marked this pull request as ready for review February 7, 2025 19:32

workingjubilee changed the title ~~WIP: compiler: gate extern "{abi}" in ast_lowering~~ compiler: gate extern "{abi}" in ast_lowering Feb 7, 2025

rustbot assigned compiler-errors Feb 7, 2025

workingjubilee force-pushed the move-abi-versioning-into-ast branch 2 times, most recently from 79d0646 to f5512f4 Compare February 7, 2025 20:24

workingjubilee force-pushed the move-abi-versioning-into-ast branch from f5512f4 to 5d7c093 Compare February 9, 2025 18:41

compiler-errors reviewed Feb 9, 2025

View reviewed changes

workingjubilee force-pushed the move-abi-versioning-into-ast branch from 1fcf247 to 6b8fab8 Compare February 10, 2025 00:54

compiler-errors approved these changes Feb 10, 2025

View reviewed changes

workingjubilee added 4 commits February 9, 2025 20:36

compiler: start using rustc_ast_lowering in rustc_passes

90c50f0

tests: error strings for ABI stability now match

ca19347

compiler: remove rustc_target::spec::abi reexports

54ff6e0

compiler: remove abi-specific extern "{abi}" suggestions

cd9d39e

These are either residue of a long-term migration away from something, or are simply trying too hard to be specifically useful: nearest-match suggestions for ABI strings should handle this.

workingjubilee force-pushed the move-abi-versioning-into-ast branch from 6b8fab8 to cd9d39e Compare February 10, 2025 04:52

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 10, 2025

workingjubilee mentioned this pull request Feb 10, 2025

Rollup of 9 pull requests #136811

Closed

matthiaskrgr mentioned this pull request Feb 10, 2025

Rollup of 8 pull requests #136834

Closed

This was referenced Feb 10, 2025

Rollup of 8 pull requests #136836

Closed

Rollup of 7 pull requests #136845

Merged

workingjubilee mentioned this pull request Feb 11, 2025

Tracking issue for refactoring the way we represent function call ABIs #119183

Open

19 tasks

bors merged commit 38f4c1f into rust-lang:master Feb 11, 2025
6 checks passed

rustbot added this to the 1.86.0 milestone Feb 11, 2025

workingjubilee deleted the move-abi-versioning-into-ast branch February 11, 2025 11:00

RalfJung reviewed Feb 18, 2025

View reviewed changes

Zalathar mentioned this pull request Mar 4, 2025

Remove unused PpMode::needs_hir #137975

Merged

		@@ -1,4 +1,4 @@
		error[E0658]: intrinsics are subject to change
		error[E0658]: rust-intrinsic ABI is an implementation detail and perma-unstable

Uh oh!

compiler: gate extern "{abi}" in ast_lowering #136603

compiler: gate extern "{abi}" in ast_lowering #136603

Uh oh!

Conversation

workingjubilee commented Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

bors commented Feb 6, 2025

Uh oh!

rustbot commented Feb 7, 2025

Uh oh!

workingjubilee commented Feb 7, 2025

Uh oh!

workingjubilee commented Feb 7, 2025

Uh oh!

bors commented Feb 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compiler-errors left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

workingjubilee Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

workingjubilee commented Feb 10, 2025

Uh oh!

bors commented Feb 10, 2025

Uh oh!

Uh oh!

RalfJung Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

compiler: gate `extern "{abi}"` in ast_lowering #136603

compiler: gate `extern "{abi}"` in ast_lowering #136603

workingjubilee commented Feb 5, 2025 •

edited

Loading

workingjubilee Feb 10, 2025 •

edited

Loading

RalfJung Feb 18, 2025 •

edited

Loading